fix: render custom Pages Router 404 for getStaticProps/getServerSideProps notFound results - #1346
fix: render custom Pages Router 404 for getStaticProps/getServerSideProps notFound results#1346rome2o wants to merge 2 commits into
Conversation
…rops notFound results
Root cause: opennextjs-cloudflare builds a bare `NextServer` (via
@opennextjs/aws's dist/core/util.js) and calls its request handler
directly, without ever running a Next.js "router-server" process.
Next.js's Pages Router route handler
(next/dist/server/route-modules/pages/pages-handler.js) falls back to
a hardcoded `"This page could not be found"` body whenever
`routerServerContext.render404` is unavailable. `routerServerContext`
is read from a global registry
(routerServerGlobal[RouterServerContextSymbol][relativeProjectDir])
that NextNodeServer only self-populates lazily, inside
`handleCatchallRenderRequest` - i.e. only once a genuinely unmatched
path is hit. Any request that matches a real page whose
getStaticProps/getServerSideProps returns `{ notFound: true }` can be
the first request handled in a Worker isolate, before that lazy
registration ever runs, so the app's actual pages/404/pages/_error is
never rendered.
Fix: patch NextServer#makeRequestHandler() (next/dist/server/next-server.js)
to perform the same self-registration unconditionally, before the
request handler is returned and therefore before any request -
matched or not - can reach the route module. This reuses
`this.render404`, the same method that already correctly renders the
custom 404 page for genuinely unmatched paths. The registration key
is hardcoded to "" (relativeProjectDir's build-time constant value)
since OpenNext always constructs NextServer with dir: "".
This mirrors the fix Cloudflare's vinext project shipped for the
equivalent Pages Router bug: rerouting `notFound` results to the app's
actual 404/error page instead of a built-in fallback.
See cloudflare/vinext#1737 and cloudflare/vinext#2773 (header
preservation follow-up).
Adds an e2e regression test (ssr-not-found) asserting the actual
404/error page body is served, not just a 404 status code.
🦋 Changeset detectedLatest commit: 9ac06a9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir].nextConfig = this.nextConfig; | ||
| _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir].isWrappedByNextServer = true; |
There was a problem hiding this comment.
🔴 Query parameters for dynamic pages may be silently dropped on every request
The patch turns on Next.js's "hosted behind a router server" flag for the whole app (isWrappedByNextServer = true at packages/cloudflare/src/cli/build/patches/plugins/next-server.ts:225) even though no router server exists here, so Next.js skips the step that turns internally-prefixed routing parameters back into real page parameters and instead deletes them.
Impact: Pages that receive route parameters through internal prefixed query values can render with missing parameters, producing wrong or empty content instead of the expected page.
Why the registration flips a query-normalization branch that was previously never taken
Next reads the context in RouteModule#prepare() using the key getRequestMeta(req, 'relativeProjectDir') || this.relativeProjectDir (route-module.js:287-288 in next@15.5.21). Under OpenNext that key is "", while Next's own lazy self-registration in handleCatchallRenderRequest writes under path.relative(process.cwd(), this.dir) — which the PR description itself observed to be ".." at runtime. So before this patch the lookup at key "" returned undefined and route-module.js:391 took the serverUtils.normalizeQueryParams(query, routeParamKeys) branch, which strips the nxtP/interception prefixes, decodes the values and re-adds them as real query keys, populating routeParamKeys.
After this patch an entry now exists at key "" with isWrappedByNextServer = true, so the other branch runs: serverUtils.filterInternalQuery(query, []), which simply deletes every nxtP-/nxti-prefixed key and leaves routeParamKeys empty. That branch is only correct when an upstream router server has already normalized the query, which is not the case for OpenNext.
If only render404 is needed for the Pages Router notFound fix, registering render404 (and optionally nextConfig) without setting isWrappedByNextServer keeps the previous query handling intact.
Prompt for agents
The new registerRouterServerContextRule in packages/cloudflare/src/cli/build/patches/plugins/next-server.ts injects a routerServerContext entry under the key "" and sets isWrappedByNextServer = true. In Next's route-module.js (RouteModule#prepare), that flag selects filterInternalQuery(query, []) instead of serverUtils.normalizeQueryParams(query, routeParamKeys). Because OpenNext has no upstream router server performing the normalization, and because before this patch the lookup at key "" always missed (Next's own lazy registration writes under path.relative(process.cwd(), this.dir), observed to be ".."), the normalizeQueryParams branch was the one always taken. Setting the flag therefore changes query handling for every request, causing nxtP-/nxti-prefixed routing params to be deleted rather than decoded into real query params, and leaving routeParamKeys empty for the subsequent dynamic-route param interpolation. Consider registering only render404 (and nextConfig if needed) without isWrappedByNextServer, and add e2e coverage for dynamic routes with rewrites/middleware to confirm params still resolve.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — fixed in 9ac06a9. Dropped the isWrappedByNextServer = true assignment; only render404 (and nextConfig, already read elsewhere in RouteModule) is needed for the notFound fix. Verified manually that /api/dynamic/[slug] still resolves params correctly with it removed, and the e2e regression test still passes.
…text Setting isWrappedByNextServer flips RouteModule#prepare() (route-module.js) from serverUtils.normalizeQueryParams(...) to serverUtils.filterInternalQuery(...), which is only correct when an upstream router-server process has already normalized the query. OpenNext has no such process, so this deleted nxtP-/nxti-prefixed routing params instead of decoding them into real page params on every request. Only render404 (and nextConfig, used elsewhere in RouteModule) is needed for the Pages Router notFound fix, so drop the isWrappedByNextServer assignment. Verified manually: /api/dynamic/[slug] resolved params correctly with the fix removed, and the ssr-not-found e2e regression test still passes. Found by Devin Review on PR opennextjs#1346.
|
@vicb - Please let me know if we need anything changed here! |
|
I'll get to that but not before next week. One question for you in the meantime: the cloudflare adapter is based off the aws adapter. Should it be fixed there instead? The repo is https://github.com/opennextjs/opennextjs-aws - it should be easy enough to test the behavior there. Thanks! |
commit: |
|
Superseded by opennextjs/opennextjs-aws#1229, which fixes this in the shared core. Not a fresh-isolate race as I originally described, it is a permanent key mismatch: Next registers the context under Verified here with the ast-grep patch disabled: pages-router e2e green, 37 passed, 1 skipped. Happy to close once #1229 merges. |
2 participants
Problem
When a Pages Router page's
getStaticProps/getServerSidePropsreturns{ notFound: true }, Next.js internally callsrouterServerContext.render404(...)to render the app's actualpages/404/pages/_error. Under@opennextjs/cloudflare, this is undefined for the first request(s) a fresh Worker isolate handles, so Next.js falls back to a bare, hardcoded"This page could not be found"body with noContent-Type, no HTML, no styling — instead of the app's actual 404 page.Root cause
opennextjs-cloudflarebuilds a bareNextServer(via@opennextjs/aws'sdist/core/util.js) and calls its request handler directly — no Next.js "router-server" process ever runs. Next.js's Pages Router route handler (next/dist/server/route-modules/pages/pages-handler.js) falls back to the hardcoded string wheneverrouterServerContext.render404is unavailable.routerServerContextis read from a global registry (routerServerGlobal[RouterServerContextSymbol][relativeProjectDir]) thatNextNodeServeronly self-populates lazily, insidehandleCatchallRenderRequest— i.e. only once a genuinely unmatched path is hit.Any request that matches a real page whose
getStaticProps/getServerSidePropsreturns{ notFound: true }can be the very first request handled by a fresh Worker isolate, before that lazy registration ever runs.Fix
Patch
NextServer#makeRequestHandler()(next/dist/server/next-server.js) to perform the same self-registration unconditionally, before the request handler is returned — so it runs before any request, matched or not, can reach the route module. This reusesthis.render404, the same method that already correctly renders the custom 404 page for genuinely unmatched paths. The registration key is hardcoded to""(relativeProjectDir's build-time constant value) since OpenNext always constructsNextServerwithdir: "".This mirrors the fix Cloudflare's
vinextproject shipped for the equivalent Pages Router bug: reroutingnotFoundresults to the app's actual 404/error page instead of a built-in fallback. See cloudflare/vinext#1737 and cloudflare/vinext#2773 (header preservation follow-up).Only
render404andnextConfigare registered — deliberately notisWrappedByNextServer(see review fix below).Testing
next-server.spec.tscovering the newregisterRouterServerContextRuleast-grep patch rule; full unit suite passes (343/343).examples/e2e/pages-router/e2e/ssr-not-found.test.ts) against agetServerSidePropsroute that always returns{ notFound: true }— deliberately chosen becausegetServerSidePropsre-runs on every request, so it can be the very first request a fresh Worker isolate handles (unlike agetStaticPropsnotFoundresult, which resolves at build time). Asserts the response is a full rendered HTML document (<!DOCTYPE html>) and not the literal bare fallback string.wrangler/workerd(not just the test harness): built and booted both the unpatched (main) and patched worker, and diffed the raw HTTP response for the same route:HTTP/1.1 404with noContent-Type, body = literalThis page could not be found(29 bytes)HTTP/1.1 404withContent-Type: text/html; charset=utf-8, full rendered_error/404 page HTML (head tags, CSS, JS chunks)isWrappedByNextServer = true, which flipsRouteModule#prepare()(route-module.js) fromserverUtils.normalizeQueryParams(...)toserverUtils.filterInternalQuery(...)— a branch that's only correct when an upstream router-server process has already normalized the query, which never happens under OpenNext. This would have silently deletednxtP-/nxti-prefixed dynamic route params on every request instead of decoding them. Removed that assignment (onlyrender404/nextConfigare needed) and manually re-verified against/api/dynamic/[slug]in thepages-routerexample: params resolve correctly ({"slug":"hello-world-123"}) with the flag removed, and the 404 fix and e2e regression test both still pass.pnpm --filter cloudflare build,ts:check, andlint:checkall clean.app-router/app-pages-routerexamples (this bug is Pages-Router-specific, but flagging as not yet covered by this PR).Changeset
Included (
patchbump for@opennextjs/cloudflare).